-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Fix niteoweb/pyramid_heroku#22] Add request_id... #38
base: main
Are you sure you want to change the base?
Conversation
... to sentry and structlog logs Signed-off-by: Sourya Vatsyayan <[email protected]>
pyramid_heroku/request_id.py
Outdated
def __call__(self, request): | ||
request_id = request.headers.get("X-Request-ID") | ||
|
||
if request_id is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Less nesting is better. So it's better to have
if not request_id:
return self.handler(request)
... business logic here ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
pyramid_heroku/request_id.py
Outdated
import sentry_sdk | ||
IS_SENTRY_ENABLED = True | ||
except ImportError: | ||
IS_SENTRY_ENABLED = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be SENTRY_INSTALLED
, not "enabled".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
pyramid_heroku/tests/__init__.py
Outdated
@@ -0,0 +1,9 @@ | |||
import unittest | |||
|
|||
from .test_herokuapp_access import TestHerokuappAccessTween |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use absolute, not relative, imports. Easier to sort, and to move around, fever problems with circular dependencies, easier for IDEs to parse, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed this file, since pytest discovers the test files. I was trying to do it with python -m unittest pyramid_heroku.tests
.
Please use this branch of pyramid_heroku in a Review App of one of the apps so that we confirm things are working in a production-like environment. |
Mocking module imports doesn't go along well with pytest. sys.modules doesn't remain the same throughout. Besides, the previous implementation used global vars in tests, which is not really recommended. Signed-off-by: Sourya Vatsyayan <[email protected]>
This brings up test coverage to 100%. B-) Also, add type hints in some functions and the global vars. Signed-off-by: Sourya Vatsyayan <[email protected]>
I don't quite understand the workflow of a review app. Is there an existing Pyramid app that I can fork to test and deploy, or should I build my own from scratch? EDIT: Using https://github.com/niteoweb/pyramid-realworld-example-app |
... to sentry and structlog logs
Signed-off-by: Sourya Vatsyayan [email protected]